-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle improper Accept headers #1795
Handle improper Accept headers #1795
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good. Fix CHANGELOG.
It would be a good idea to add some unit tests that include actual calls to vendor?
and such after parsing an invalid accept header.
CHANGELOG.md
Outdated
@@ -3,6 +3,7 @@ | |||
#### Features | |||
|
|||
* Your contribution here. | |||
* [#1795](https://github.com/ruby-grape/grape/pull/1795): Fix Accept header parsing bug - [@bschmeck](https://github.com/bschmeck) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a period at the end. It could also be more specific: "Fix: vendor/subtype parsing in an invalid Accept header"
Pushed a correction to the CHANGELOG, I had missed the comment from grape-bot.
|
Thanks, makes sense. I would refactor this and take parsing out of the header middleware implementation, and then we can have tests, but that's beyond the scope of this PR. |
https://travis-ci.org/ruby-grape/grape/jobs/434730968 has a failure, I am not sure what's up with that, care to check it out? |
I will try to find some time to work on extracting the parsing logic and wrapping it in tests. I'll dig into the failure, too. It appears to be only on |
I am sure it's unrelated, but we do need a green build one way or another. |
a3dee70
to
c576a88
Compare
c576a88
to
50135b0
Compare
The upstream problem in ActiveSupport was fixed and the build is now green for this PR. |
Thanks, merged. |
We have observed situations where our API has received a request from a network scanner that has an improperly formatted Accept header. In these cases,
Rack:Accept::Header.parse_media_type
returns an empty array (https://github.com/mjackson/rack-accept/blob/master/lib/rack/accept/header.rb#L42) andsubtype
is set tonil
.This PR checks that we actually received a subtype before checking against the vendor and version regexes.